[ContainerRegistry] Update to beta.2 API design by jeremymeng · Pull Request #15109 · Azure/azure-sdk-for-js

jeremymeng · 2021-04-30T22:53:08Z

and address JS review feedback. API changes include

Change to a single ContainerRegistryClient with helper classes of hierarchy ContainerRepository => RegistryArtifact to group repository and artifact operations
Add string literal types for artifact architectures and operation systems.
Combine the parameters to Set*Properties() methods
Add readonly properties like loginServer and fullyQualifiedName for better interaction with docker and other tools.
Various renames

for `set*Properties()` methods.

- type alias for known architecture and operating systems. - orderby literals: "timeAsc" and "timeDesc"

Fix linting issue Update api review md file

jeremymeng · 2021-05-05T21:35:29Z

@chradek @xirzec please take a look

xirzec

Some initial feedback on the api shapes

xirzec · 2021-05-06T22:36:33Z

 ```ts

+import * as coreClient from '@azure/core-client';
+import * as coreHttps from '@azure/core-rest-pipeline';


did we intend to alias this as a group? I notice we directly import PipelineOptions below

I believe these two are from the generated code and probably pulled in by api-extractor when we export generated types.

Somehow it's gone in the latest version, after I removed export of a generated type.

xirzec · 2021-05-06T22:37:07Z

 import { TokenCredential } from '@azure/core-auth';

+// @public
+export type ArtifactArchitecture = "386" | "amd64" | "arm" | "arm64" | "mips" | "mipsle" | "mips64" | "mips64le" | "ppc64" | "ppc64le" | "riscv64" | "s390x" | "wasm" | string;


does this type provide value? TS will collapse this to type ArtifactArchitecture = string

Right, I was thinking about making them extensible, but the generated code has export const enum KnownArtifactArchitecture which I don't think we like in TS.

Changed to string, and export KnownArtifactArchitecture literal unions.

xirzec · 2021-05-06T22:37:45Z

+
+// @public
+export interface ArtifactManifestProperties {
+    readonly architecture?: ArtifactArchitecture | null;


what does null vs undefined mean? Does it remove the property at the source when doing a patch/update?

question applies to other nullable properties too

These are output only. It's an error from copy-&-pasting the generated code.

Removed | null

xirzec · 2021-05-06T22:38:24Z

+    readonly operatingSystem?: ArtifactOperatingSystem | null;
+    readonly repositoryName?: string;
+    readonly size?: number;
+    readonly tags?: string[];


can we make this default to an empty array similar to manifests?

xirzec · 2021-05-06T22:38:31Z

+}
+
+// @public
+export type ArtifactOperatingSystem = "aix" | "android" | "darwin" | "dragonfly" | "freebsd" | "illumos" | "ios" | "js" | "linux" | "netbsd" | "openbsd" | "plan9" | "solaris" | "windows" | string;


same issue as above

Aliased it to string

xirzec · 2021-05-06T22:40:08Z

+    getArtifact(repositoryName: string, tagOrDigest: string): RegistryArtifact;
+    getRepository(repositoryName: string): ContainerRepository;
+    listRepositoryNames(options?: ListRepositoriesOptions): PagedAsyncIterableIterator<string, string[]>;
+    loginServer: string;


this are public, mutable properties?

Should be readonly

xirzec · 2021-05-06T22:41:13Z

-export class ContainerRepositoryClient {
-    constructor(endpointUrl: string, repository: string, credential: TokenCredential, options?: ContainerRegistryClientOptions);
+export class ContainerRepository {
+    // Warning: (ae-forgotten-export) The symbol "GeneratedClient" needs to be exported by the entry point index.d.ts


this feels a bit leaky, we should be able to avoid exposing GeneratedClient -- seems like this class should be hidden and we can expose a compatible ContainerRegistry interface that we return the class instance for

Yes I will expose interface instead.

xirzec · 2021-05-06T22:41:49Z

+    readonly deletedManifests?: string[];
+    readonly deletedTags?: string[];


would be nice to default these arrays to empty array

xirzec · 2021-05-06T22:42:17Z

-    writeableProperties?: ContentProperties;
+export class RegistryArtifact {
+    // @internal
+    constructor(registryUrl: string, repositoryName: string, tagOrDigest: string, client: GeneratedClient);


same thinking, let's make this an interface since it's not a Client

xirzec · 2021-05-06T22:42:27Z

+    constructor(registryUrl: string, repositoryName: string, tagOrDigest: string, client: GeneratedClient);
+    delete(options?: DeleteArtifactOptions): Promise<void>;
+    deleteTag(tag: string, options?: DeleteTagOptions): Promise<void>;
+    // (undocumented)


xirzec

Reviewed notes + api shape and both look good to me modulo some comments. 👍

I didn't review changes to other files, so probably good to get one more reviewer signoff before merging.

xirzec · 2021-05-10T18:43:44Z

 // @public
 export interface ArtifactManifestProperties {
-    readonly architecture?: ArtifactArchitecture | null;
+    readonly architecture?: ArtifactArchitecture;


since this is just string now, can we make it string here instead of having to define a type that aliases string?

xirzec · 2021-05-10T18:43:59Z

    readonly digest?: string;
    readonly lastUpdatedOn?: Date;
-    manifests?: ArtifactManifestProperties[];
-    readonly operatingSystem?: ArtifactOperatingSystem | null;


same thinking here

Improve CHANGELOG Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

sadasant · 2021-05-10T20:22:59Z

+        ? "timeasc"
+        : options.orderBy === "timeDesc"
+        ? "timedesc"
+        : undefined;


I wonder if toLowerCase would be better here. It's not really that relevant in itself, however: These constant values, would they change later? should we scope these transformations in some functions? so that the transformation changes happen separately from the logic changes?

Take it or leave it, I'm just sharing an idea 🌞

Moved to a transformation helper function.

sadasant · 2021-05-10T20:27:39Z

+      }
+      if (currentPage.manifests) {
+        yield currentPage.manifests.map((t) => {
+          return {


In Key Vault we like to put translations like this (type A to type B) in a "transformations" file. What we find important is not the name of the file, but the idea of separating the renames from the logic around the renames. Sometimes these renames happen more than once, so these functions can be re-used. Here's one for KV Keys: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/keyvault/keyvault-keys/src/transformations.ts

I extracted this into a helper function in transformation.ts

sadasant

Thank you Jeremy!, this is good work 👍

jeremymeng added 8 commits April 27, 2021 16:45

Run codegen for beta2 swagger

df7a606

Adapt to beta.2 API design

4aa9e74

Combine value parameter with options parameter

0f96467

for `set*Properties()` methods.

Update published samples

af8240d

React to swagger updates

3e9933e

Tweak APIs

f64d203

- type alias for known architecture and operating systems. - orderby literals: "timeAsc" and "timeDesc"

Rename test descriptions

cad9d90

Rename parameter to repositoryName

330491f

Fix linting issue Update api review md file

jeremymeng requested review from bterlson, chradek and xirzec as code owners April 30, 2021 22:53

jeremymeng requested review from annelo-msft, pallavit and seankane-msft April 30, 2021 22:53

jeremymeng added Container Registry Client This issue points to a problem in the data-plane of the library. labels May 4, 2021

jeremymeng mentioned this pull request May 4, 2021

Standardize on repositoryName parameter in ConainerRegistryClient methods #15132

Closed

xirzec reviewed May 6, 2021

View reviewed changes

jeremymeng added 3 commits May 7, 2021 12:52

Make properties of nullable array empty array instead

f3bce0e

Address review feedback

13ae775

More readonly properties

2d3744d

jeremymeng mentioned this pull request May 7, 2021

Implement ACR API Updates #14965

Closed

4 tasks

Update changelog

a5c673e

xirzec approved these changes May 10, 2021

View reviewed changes

jeremymeng and others added 3 commits May 10, 2021 12:00

Apply suggestions from code review

113ab39

Improve CHANGELOG Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

Update published samples after API change

93c9fb4

Remove aliased types for os and arch

b962a1a

sadasant reviewed May 10, 2021

View reviewed changes

Comment thread sdk/containerregistry/container-registry/src/containerRegistryClient.ts

sadasant reviewed May 10, 2021

View reviewed changes

Comment thread sdk/containerregistry/container-registry/src/containerRepository.ts

sadasant reviewed May 10, 2021

View reviewed changes

Comment thread sdk/containerregistry/container-registry/src/containerRepository.ts

sadasant reviewed May 10, 2021

View reviewed changes

jeremymeng added 2 commits May 10, 2021 14:14

refactoring: extract helper transformation functions

11d7d2e

Update samples and docs

4ee1adf

sadasant approved these changes May 10, 2021

View reviewed changes

jeremymeng merged commit 5eff9b8 into Azure:master May 10, 2021

jeremymeng deleted the acr-beta2-swagger branch May 10, 2021 22:26

		readonly deletedManifests?: string[];
		readonly deletedTags?: string[];

Conversation

jeremymeng commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremymeng commented May 5, 2021

Uh oh!

xirzec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xirzec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremymeng commented Apr 30, 2021 •

edited

Loading